Add memory64 and table64 support to the Canonical ABI#624
Add memory64 and table64 support to the Canonical ABI#624lukewagner merged 58 commits intoWebAssembly:mainfrom
Conversation
Parameterize the Canonical ABI to handle 32-bit or 64-bit memory addresses and table indices. This is done by adding two new fields to `LiftOptions` to indicate if the `memory64`/`table64` feature is being used in a core module.
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks so much for helping to pick this up! We chatted a bit on Zulip but for posterity I'll mention here about table-related index types how most of them want to remain 32-bits. I'll take a closer look once that's been passed over here, and in the meantime I skimmed over things and noted a few things here and there. I'm sure though that @lukewagner will have thoughts on this too!
design/mvp/Explainer.md
Outdated
| (param $originalSize i32) | ||
| (func (param $originalPtr $addr) | ||
| (param $originalSize $addr) | ||
| (param $alignment i32) |
There was a problem hiding this comment.
For this I might recommend making alignment have type $addr as well for consistency, and I think that matches the signature in Rust/C/etc as well.
design/mvp/Explainer.md
Outdated
| | -------------------------- | ------------------------ | | ||
| | Approximate WIT signature | `func<T>(t: T) -> T.rep` | | ||
| | Canonical ABI signature | `[t:i32] -> [i32]` | | ||
| | Canonical ABI signature | `[t:$idx] -> [i32]` | |
There was a problem hiding this comment.
This'll be a bit subtle, but this'll actually want to be a mapping of i32 as an argument (I talked with you a bit about this already, but the input here is a host-side index so always 32-bit), but the output here should be something variable. Resource "rep"s are generally pointers in linear memory so 64-bit components are going to want 64-bit storage values. In the component model resources are defined with (rep i32) and validation currently requires that i32 is the only one listed there, but this PR will relax that meaning that the resource type itself stores the lowered type which'll get plumbed here.
There was a problem hiding this comment.
Thanks, I've now changed it so that the types which are expected to be pointers to memory are allowed to be either i32 or i64 without forcing them to match the memory type (e.g. the return value here, or in context.{get,set}, or thread.new-indirect). This means we don't need to require the memory canonopt anywhere where it didn't previously exist.
alexcrichton
left a comment
There was a problem hiding this comment.
Looks good to me! I think this'll need minor adjustments over time but overall looks good 👍
| if opts.memory is None: | ||
| return 'i32' |
There was a problem hiding this comment.
Could this have an assert? This in theory shouldn't ever be called dynamically if memory is None
design/mvp/CanonicalABI.md
Outdated
| * `memory` - this is a subtype of `(memory 1)`. In the rest of the explainer, | ||
| `PTR` will refer to either `i32` or `i64` core Wasm types as determined by the | ||
| type of this `memory`. |
There was a problem hiding this comment.
This'll technically want to say it's a subtype of memory 1 or memory i64 1 since those are separate types
design/mvp/CanonicalABI.md
Outdated
| feature (the "stackful" ABI), this restriction is lifted. | ||
| * 🔀 `callback` - the function has type `(func (param i32 i32 i32) (result i32))` | ||
| and cannot be present without `async` and is only allowed with | ||
| * 🔀 `callback` - the function has type `(func (param i32 i32 PTR) (result i32))` |
There was a problem hiding this comment.
I think this one may remain a three i32s since they're all async-related codes/events/etc.
lukewagner
left a comment
There was a problem hiding this comment.
This looks great overall; thanks so much for all the work and adding tests too! There's only one relatively minor, but non-nit, suggestion below that I'm happy to discuss and then re-review based on what we decide.
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
0b5e62d to
4fd7d8b
Compare
We should start work in the next week on implementation. |
lukewagner
left a comment
There was a problem hiding this comment.
Looks good to me, thanks! Now that things are gated, I'll merge once these nits are fixed.
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
|
Thanks again @lukewagner ! |
|
And thank you again for working on this! |
Parameterize the Canonical ABI to handle 32-bit or 64-bit memory addresses and table indices. This is done by adding two new fields to
LiftOptionsto indicate if thememory64/table64feature is being used in a core module.